feat: hb_device reserved keys - #1077
Conversation
|
|
||
| %% @doc Check if a key is claimed by the device's direct function dispatch. | ||
| is_device_key(Dev, Base, Key, Opts) -> | ||
| case message_to_fun(Dev, Base, Key, Opts) of |
There was a problem hiding this comment.
We should handle the case where there is no match? Conditions for that to happen the device author would need to halt the device from defaulting to ~message@1.0 and also not implement a catch all handler themselves.
There was a problem hiding this comment.
There is an issue where info_handler_to_fun calls message_to_fun, which returns a tuple with 3 values, but the result is expected to be a tuple with 2 values.
{Status, Func} = info_handler_to_fun(Handler, Msg, Key, Opts),I will fix this and also catch the throw to avoid the infinity loop.
| is_exported(_Info, _Key, _Opts) -> true. | ||
|
|
||
| %% @doc Determine if a key is reserved by a device. | ||
| %% A key is reserved if it is declared in the device's `info().reserved' list, |
There was a problem hiding this comment.
Seems OK but its a kind of strange concept when you think about it... What exactly does it mean for the key to be reserved vs exported? We don't expect there to be any other protocol rules, but your idea is to have a way for a message's device to signal generically that while a key not be computed it... Should also not be set by the user? But it may be found as raw data because the device itself can/will set it? In which case it is part of the device's public interface -- just not computed directly?
If so, I think I grok it. Comment in that case is technically correct but maybe we should add a tiny sentence explaining 'this allows device authors to signal generically to other callers that a given key in a message is anticipated to be used for the device's internal purposes, whether or not its value is defined by Erlang function calls or message@1.0-inherited literal lookups.'?
| explicit_reserved_key_test() -> | ||
| Trie = #{ <<"device">> => <<"trie@1.0">> }, | ||
| ?assert(is_reserved(Trie, <<"node-value">>, #{})), | ||
| ?assert(is_reserved(<<"trie@1.0">>, Trie, <<"node-value">>, #{})), | ||
| ?assert(is_reserved(Trie, <<"Node-Value">>, #{})), | ||
| ?assertNot(is_reserved(Trie, <<"alice">>, #{})). | ||
|
|
||
| message_reserved_key_test() -> | ||
| Msg = #{ <<"device">> => <<"message@1.0">> }, | ||
| Trie = #{ <<"device">> => <<"trie@1.0">> }, | ||
| ?assert(is_reserved(Msg, <<"path">>, #{})), | ||
| ?assert(is_reserved(Msg, <<"set">>, #{})), | ||
| ?assert(is_reserved(Msg, <<"get">>, #{})), | ||
| ?assert(is_reserved(Trie, <<"get">>, #{})), | ||
| ?assert(is_reserved(Trie, <<"set">>, #{})), | ||
| ?assert(is_reserved(Trie, <<"keys">>, #{})), | ||
| ?assertNot(is_reserved(Trie, <<"commit">>, #{})), | ||
| ?assertNot(is_reserved(Msg, <<"alice">>, #{})). | ||
|
|
||
| trie_keys_skip_reserved_keys_test() -> | ||
| Trie = | ||
| #{ | ||
| <<"device">> => <<"trie@1.0">>, | ||
| <<"node-value">> => ignored, | ||
| <<"get">> => ignored, | ||
| <<"set">> => ignored, | ||
| <<"keys">> => ignored, | ||
| <<"alice">> => 1 | ||
| }, | ||
| ?assertEqual([<<>>, <<"alice">>], lists:sort(hb_ao:keys(Trie, #{}))). |
There was a problem hiding this comment.
Its a nit but mind using ~test-device@1.0 for this please? We might move the trie@1.0 out of the preloaded devices sometime.
| <<"verify">> | ||
| ]). | ||
|
|
||
| %% The list of keys that the message device reserves at protocol level. |
There was a problem hiding this comment.
I think this shouldn't be needed? These should all be returned as true by is_reserved anyway?
| <<"node-value">>, | ||
| <<"device">>, | ||
| <<"commitments">>, | ||
| <<"priv">>, |
There was a problem hiding this comment.
Shouldn't be an addressable key anyway?
| [ | ||
| Key | ||
| || | ||
| Key <- hb_maps:keys(TrieNode, Opts), |
There was a problem hiding this comment.
You probably want hb_private:reset here before the keys invocation?
3 participants
this PR add
hb_device:is_reserved/3,4: a key is reserved if the device declares it ininfo().reservedor it resolves to a real exported device functionit lets callers validate user keys against the actual device namespace, so
trie@1.0rejects internals likenode-valueand callable keys likeget/setwithout having a hardcoded?RESERVED_KEYSmacro (removed in this PR)